-
Notifications
You must be signed in to change notification settings - Fork 112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix find_idx
#567
Fix find_idx
#567
Conversation
Is the version number messed? Latest document sad v1.9.1+, but release notes have been v1.9.3+ |
Do we need to worry following warnings now? tests/test_case.py::TestPlot::test_kundur_plot
/Users/jinningwang/work/andes/andes/utils/lazyimport.py:80: DeprecationWarning: Use shutil.which instead of find_executable
return eval(self.__imported_name__)(*args, **kwargs)
tests/test_case.py::TestCaseInit::test_pvd1_init
/Users/jinningwang/work/andes/andes/core/model/model.py:754: ComplexWarning: Casting complex values to real discards the imaginary part
instance.v = np.array(func(*self.s_args[name]),
tests/test_case.py::TestCaseInit::test_pvd1_init
/Users/jinningwang/work/andes/andes/core/model/model.py:793: ComplexWarning: Casting complex values to real discards the imaginary part
instance.v[:] = func(*self.s_args[name])
tests/test_known_good.py: 12 warnings
/Users/jinningwang/work/miniconda3/envs/ams/lib/python3.10/site-packages/dill/_dill.py:1028: DeprecationWarning: numpy.core is deprecated and has been renamed to numpy._core. The numpy._core namespace contains private NumPy internals and its use is discouraged, as NumPy internals can change without warning in any release. In practice, most real-world usage of numpy.core is to access functionality in the public NumPy API. If that is the case, use the public NumPy API. If not, you are using NumPy internals. If you would still like to access an internal attribute, use numpy._core._multiarray_umath.
submodule = getattr(__import__(module, None, None, [obj]), obj) |
Docker_hub integration needs attentation, https://github.com/CURENT/andes/actions/runs/11077636077/job/30783344184?pr=567 ERROR: failed to solve: python:3.9-slim: failed to resolve source metadata for docker.io/library/python:3.9-slim: failed to authorize: failed to fetch oauth token: unexpected status from GET request to https://auth.docker.io/token?scope=repository%3Alibrary%2Fpython%3Apull&service=registry.docker.io: 401 Unauthorized
Error: Docker build failed with exit code 1 |
Jinning, The warnings can be fixed later, but can you bump up the Python image version in the GitHub actions file to fix the Docker error? |
The trouble with github action seems come from the action setup-miniconda@v3 itself: /home/runner/miniconda3/condabin/conda config --add channels defaults
Warning: Error while loading conda entry point: conda-libmamba-solver (module 'libmambapy' has no attribute 'QueryFormat') Leave it here for now. |
andes/models/group.py
Outdated
@@ -243,31 +243,41 @@ def set(self, src: str, idx, attr, value): | |||
|
|||
return True | |||
|
|||
def find_idx(self, keys, values, allow_none=False, default=None): | |||
def find_idx(self, keys, values, allow_none=False, default=None, | |||
no_flatten=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no_flatten=False
introduces more ambiguity. Remove it since we don't actually need it.
Enforce the return to be a list of lists, thus the behavior is more consistent across models and groups.
andes/models/group.py
Outdated
@@ -243,31 +243,41 @@ def set(self, src: str, idx, attr, value): | |||
|
|||
return True | |||
|
|||
def find_idx(self, keys, values, allow_none=False, default=None): | |||
def find_idx(self, keys, values, allow_none=False, default=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default=None
can be kept there to be consistent with ModelData.find_idx()
I'd like to ask for your suggestions for the below issue. Problemsometimes the Minimal code>>> raw = andes.get_case("kundur/kundur.raw")
>>> dyr = andes.get_case("kundur/kundur_full.dyr")
>>> ss = andes.load(raw, addfile=dyr, setup=False)
>>> ss.GENROU.gen.v
[[1], [2], [3], [4]] Solution optionsOption 1: Add
|
for model_idx, dest_idx in zip(model.idx.v, idxp.v): | |
if dest_idx not in dest.uid: | |
continue |
Comment: IdxParam.v
can still be list of lists that breaks the existing users' expectation
Option 3: Add a sanitize step to IdxParam.v
using @v.setter
Comment: Seems to be a good choice to balance the complexity and consistency
This seems to be a result of the change to “find_idx”. It reminds me that the caller to that function is assuming unique idx. This is the case in GENRIU.gen.
To minimize the change in code, maybe we should revert the change to make it behave the old way. You can add a keyword argument, to allow finding all the indices if needed. This will solve the original problem but not impact the existing code.
Get Outlook for iOS<https://aka.ms/o0ukef>
________________________________
From: Jinning Wang ***@***.***>
Sent: Tuesday, October 1, 2024 9:52:13 AM
To: CURENT/andes ***@***.***>
Cc: Hantao Cui ***@***.***>; Mention ***@***.***>
Subject: Re: [CURENT/andes] Fix `find_idx` (PR #567)
@cuihantao<https://github.com/cuihantao>
I'd like to ask for your suggestions for the below issue.
Problem
sometimes the IdxParam.v can be list of lists, which results in error in System.setup() as the IdxParam.v is expected to be a flatten list.
Minimal code
>> raw = andes.get_case("kundur/kundur.raw")
>> dyr = andes.get_case("kundur/kundur_full.dyr")
>> ss = andes.load(raw, addfile=dyr, setup=False)
>> ss.GENROU.gen.v
[[1], [2], [3], [4]]
Solution options
Option 1: Add no_flatten=Falses in find_idx()
Comment: might be confusing as there can be one-to-many mapping
Option 2: Sanitize impacted code (shown below):
https://github.com/CURENT/andes/blob/5ab784b75347a98d07c004e32d7c4b39d407ea2e/andes/system.py#L1768-L1770
Comment: IdxParam.v can still be list of lists that breaks the existing users' expectation
Option 3: Add a sanitize step to IdxParam.v using @v.setter
Comment: Seems to be a good choice to balance the complexity and consistency
—
Reply to this email directly, view it on GitHub<#567 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABSNZA7YRS56KM52MRVCBCTZZKSI3AVCNFSM6AAAAABO7377CWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOBWGAYDSNBYHA>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
This issue seems to have been fixed, conda-incubator/setup-miniconda#366 Additionally, local tests show it works when using |
@cuihantao Finally this issue is fixed, I think it's ready to merge |
Thanks Jinning! Just merged it. |
ModelData.find_idx()
andGroupBase.find_idx()